Fix crash in execve filename parameter parsing - #98
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughString parameter conversion in ChangesString parameter length fallback
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Event parameter data can have a recorded length that doesn't match the null-terminated string length. This has been observed on some kernels for both string and integer parameters (e.g. clone3 exe param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). Use the full parameter length instead of throwing sinsp_exception, which propagated uncaught through sinsp::next() and crashed the collector via std::unexpected() -> abort().
9d74644 to
50ee906
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@userspace/libsinsp/event.h`:
- Around line 216-223: Update the tests around invalid_string_len and
advance_ts_get_event so an embedded-NUL length mismatch is accepted and
processed successfully instead of expecting an exception. Preserve coverage for
inputs that remain invalid by adding a separate test for a missing terminator or
zero recorded length, using the existing event-parameter test helpers and
assertions.
- Around line 216-223: The PT_CHARBUF formatting path around the
parameter-length fallback must not pass a non-NUL-terminated buffer via s.data()
to %s. Update the formatting logic used by event.cpp to use a length-aware
formatter with the string view’s size, or reject the no-NUL case before
formatting, while preserving normal NUL-terminated string handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5d55e7b2-af52-4d9a-afb6-6fbd85956912
📒 Files selected for processing (1)
userspace/libsinsp/event.h
| // We expect the parameter to be exactly one null-terminated string. | ||
| // When it doesn't match, use the full parameter length instead. | ||
| // Event parameter data can have a recorded length that doesn't match | ||
| // the null-terminated string length. This has been observed on some | ||
| // kernels for both string and integer parameters (e.g. clone3 exe | ||
| // param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). | ||
| if(param_len != string_len + 1) { | ||
| // By moving this error string building operation to a separate function | ||
| // the compiler is more likely to inline this entire function. | ||
| param.throw_invalid_len_error(string_len + 1); | ||
| string_len = param_len - 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update invalid_string_len for the new contract.
userspace/libsinsp/test/events_param.ut.cpp Lines 515-539 corrupt an embedded NUL and expect advance_ts_get_event to throw. This branch now accepts that mismatch, so the test still asserts the removed exception path. Change it to assert successful processing. Add a separate test for missing terminators or zero recorded lengths if those inputs remain invalid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@userspace/libsinsp/event.h` around lines 216 - 223, Update the tests around
invalid_string_len and advance_ts_get_event so an embedded-NUL length mismatch
is accepted and processed successfully instead of expecting an exception.
Preserve coverage for inputs that remain invalid by adding a separate test for a
missing terminator or zero recorded length, using the existing event-parameter
test helpers and assertions.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 'strnlen|param_len|get_event_param_as' userspace/libsinsp/event.h
rg -n -C 8 'as<std::string_view>|snprintf.*%s' userspace/libsinsp/event.cppRepository: stackrox/falcosecurity-libs
Length of output: 9343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- event.h relevant declarations and implementation ---'
ast-grep outline userspace/libsinsp/event.h
sed -n '70,185p' userspace/libsinsp/event.h
sed -n '189,243p' userspace/libsinsp/event.h
echo '--- data_and_len_with_legacy_null_encoding definitions/usages ---'
rg -n -C 12 'data_and_len_with_legacy_null_encoding|sinsp_evt_param::sinsp_evt_param|m_data|m_len' userspace/libsinsp userspace/libscap test 2>/dev/null | head -n 320
echo '--- formatter callers and parameter types ---'
sed -n '820,900p' userspace/libsinsp/event.cpp
rg -n -C 8 'PT_CHARBUF|as<std::string_view>|as<std::string>' userspace/libsinsp userspace/libscap test 2>/dev/null | head -n 360
echo '--- tests around mismatched lengths ---'
sed -n '470,560p' userspace/libsinsp/test/events_param.ut.cppRepository: stackrox/falcosecurity-libs
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- event parameter construction and loading ---'
rg -n -C 18 'sinsp_evt_param\(|m_params\.|load_params|get_param\(' userspace/libsinsp/event.h userspace/libsinsp/event.cpp userspace/libsinsp/*.cpp | head -n 420
echo '--- raw event/scap length definitions and parser code ---'
rg -n -C 16 'PPM_EVENT_HEADER|len.*param|param.*len|scap_evt|nparams|parse.*param|next.*param' userspace/libsinsp userspace/libscap | head -n 500
echo '--- tests for string conversion and malformed lengths ---'
rg -n -C 12 'strnlen|std::string_view|mismatched|param_len|invalid_len|as<std::string' userspace/libsinsp/test test 2>/dev/null | head -n 500
echo '--- standalone bounded-scan and formatter probe ---'
python3 - <<'PY'
import ctypes
libc = ctypes.CDLL(None)
libc.strnlen.argtypes = [ctypes.c_char_p, ctypes.c_size_t]
libc.strnlen.restype = ctypes.c_size_t
cases = [
(b"abc\0", 4),
(b"abc", 3),
(b"abc\0XYZ", 7),
(b"\0", 1),
]
for data, length in cases:
n = libc.strnlen(data, length)
print({"data": data, "param_len": length, "strnlen": n,
"fallback_len": length - 1 if length != n + 1 else n,
"has_nul": n < length})
PY
cat >/tmp/probe_percent_s.c <<'C'
`#include` <stdio.h>
`#include` <string.h>
int main(void) {
char data[] = {'a','b','c','X','Y','Z','\0'};
char out[32];
int n = snprintf(out, sizeof out, "%s", data);
printf("formatted=%s length=%zu count=%d\n", out, strlen(out), n);
return 0;
}
C
cc /tmp/probe_percent_s.c -o /tmp/probe_percent_s
/tmp/probe_percent_sRepository: stackrox/falcosecurity-libs
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -u
echo '--- event.h event loading ---'
sed -n '560,635p' userspace/libsinsp/event.h
echo '--- event.cpp parameter APIs ---'
rg -n -C 20 'get_param_as_str|get_param\(|load_params|sinsp_evt_param\(' userspace/libsinsp/event.cpp userspace/libsinsp/event.h || true
echo '--- parameter parser symbols ---'
rg -n -C 12 'm_params|ppm_param_info|param_info|nparams|event.*param|param.*event' userspace/libsinsp/event.cpp userspace/libsinsp/event.h | head -n 420 || true
echo '--- exact parameter tests ---'
rg -n -C 18 'throw|invalid|mismatch|param_len|strnlen|as<std::string_view>|as<std::string>' userspace/libsinsp/test/events_param.ut.cpp || true
echo '--- capture format and event length declarations ---'
rg -n -C 10 'struct scap_evt|scap_evt.*len|len.*scap_evt|PPM_EVENT_HEADER|ppm_evt_hdr' userspace/libscap userspace/libsinsp | head -n 320 || true
echo '--- standalone bounded-scan probe ---'
python3 - <<'PY'
import ctypes
libc = ctypes.CDLL(None)
libc.strnlen.argtypes = [ctypes.c_char_p, ctypes.c_size_t]
libc.strnlen.restype = ctypes.c_size_t
for data, length in [(b"abc\0", 4), (b"abc", 3), (b"abc\0XYZ", 7), (b"\0", 1)]:
n = libc.strnlen(data, length)
print(f"data={data!r} param_len={length} strnlen={n} "
f"has_nul={n < length} fallback_len={length - 1 if length != n + 1 else n}")
PY
cat >/tmp/probe_percent_s.c <<'C'
`#include` <stdio.h>
`#include` <string.h>
int main(void) {
char data[] = {'a','b','c','X','Y','Z','\0'};
char out[32];
int n = snprintf(out, sizeof out, "%s", data);
printf("formatted=%s length=%zu count=%d\n", out, strlen(out), n);
return 0;
}
C
cc /tmp/probe_percent_s.c -o /tmp/probe_percent_s
/tmp/probe_percent_sRepository: stackrox/falcosecurity-libs
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -u
echo '--- decoder locations ---'
rg -l 'scap_event_decode_params' . | head -n 80
echo '--- decoder implementation and declarations ---'
for f in $(rg -l 'scap_event_decode_params' userspace); do
echo "### $f"
rg -n -C 35 'scap_event_decode_params' "$f" || true
done
echo '--- focused tests ---'
sed -n '480,555p' userspace/libsinsp/test/events_param.ut.cpp
rg -n -C 14 'std::string_view|as<std::string>|param_len|strnlen|throw_invalid_len' userspace/libsinsp/test/events_param.ut.cpp || true
echo '--- string parameter metadata ---'
rg -n -C 8 'PT_CHARBUF|PT_FSPATH|PT_FSRELPATH' driver userspace/libscap userspace/libsinsp | head -n 360 || true
echo '--- changed-file diff summary and nearby test changes ---'
git diff --stat -- userspace/libsinsp/event.h userspace/libsinsp/test/events_param.ut.cpp
git diff -- userspace/libsinsp/event.h userspace/libsinsp/test/events_param.ut.cpp | sed -n '1,260p'Repository: stackrox/falcosecurity-libs
Length of output: 41125
🏁 Script executed:
#!/bin/bash
set -u
echo '--- driver string length and ring encoding ---'
sed -n '450,555p' driver/ppm_events.c
rg -n -C 18 'ppm_strncpy_from_user|strnlen\(args->str_storage|val_to_ring\(args' driver/ppm_fillers.c driver/ppm_events.c | head -n 360
echo '--- event buffer ownership and allocation boundaries ---'
rg -n -C 16 'scap_evt.*len|m_pevt_storage|from_scap_evt|event_size|malloc.*evt|realloc.*evt' userspace/libscap userspace/libsinsp | head -n 420
echo '--- PT_CHARBUF formatter context ---'
sed -n '697,890p' userspace/libsinsp/event.cpp
echo '--- test event construction helper declarations ---'
rg -n -C 16 'add_event\(|add_event_advance_ts|scap_const_sized_buffer|scap_event_encode' userspace/libsinsp/test/events_param.ut.cpp test/libscap/test_suites/userspace/scap_event.cpp | head -n 360Repository: stackrox/falcosecurity-libs
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -u
echo '--- malformed-length test call path ---'
rg -n -C 24 'advance_ts_get_event|invalid_string_len|EXPECT_THROW' userspace/libsinsp/test userspace/libsinsp | head -n 420 || true
echo '--- all direct PT_CHARBUF string formatting paths ---'
rg -n -C 8 'as<std::string_view>|"%s".*s\.data|snprintf.*s\.data' userspace/libsinsp userspace/libscap | head -n 300 || true
echo '--- decoder boundary and sentinel definitions ---'
sed -n '88,132p' userspace/libscap/scap_event.c
rg -n -C 10 'PPM_ENABLE_SENTINEL|sentinel_begin|sentinel_end|struct ppm_evt_hdr' driver/ppm_events_public.h userspace/libscap | head -n 320 || trueRepository: stackrox/falcosecurity-libs
Length of output: 50383
Make PT_CHARBUF formatting length-aware.
Malformed or capture-file input can produce strnlen(param_data, param_len) == param_len. The fallback then returns a std::string_view without a NUL, but userspace/libsinsp/event.cpp:877 passes s.data() to %s. The destination bound does not limit source reads. Use a length-aware formatter or reject the no-NUL case before formatting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@userspace/libsinsp/event.h` around lines 216 - 223, The PT_CHARBUF formatting
path around the parameter-length fallback must not pass a non-NUL-terminated
buffer via s.data() to %s. Update the formatting logic used by event.cpp to use
a length-aware formatter with the string view’s size, or reject the no-NUL case
before formatting, while preserving normal NUL-terminated string handling.
Use lenient extraction instead of asstd::string_view() which throws when BPF probe data has unexpected trailing bytes after the null terminator. The uncaught exception propagates through sinsp::next() and aborts the collector.